-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test file formats #1392 #1768
Test file formats #1392 #1768
Conversation
/test connector=source-file
|
…supported file formats
@vitaliizazmic I added custom integration tests to CI -- can you make sure those tests are succeeding? you can also run them locally by running |
…nd aws providers test
…' into vitalii/1392_test_file_formats
@vitaliizazmic could you pull in @eugene-kulak 's changes from #1712 and ping me when this is ready for review? All the changes he needs to make are stylistic so I think you should be able to build on top of it. One thing to keep in mind is that you should probably put your integration tests in a separate test file for easier reading. |
# Conflicts: # airbyte-integrations/connectors/source-file/build.gradle # airbyte-integrations/connectors/source-file/integration_tests/integration_source_test.py # airbyte-integrations/connectors/source-file/setup.py # docs/integrations/sources/file.md
/test connector=source-file
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small comments, will merge tomorrow
docs/integrations/sources/file.md
Outdated
@@ -67,7 +67,7 @@ File Formats are mostly enabled \(and further tested\) thanks to other open-sour | |||
| :--- | :--- | | |||
| CSV | Yes | | |||
| JSON | Experimental | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this one still listed as experimental?
@@ -0,0 +1,21 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we don't support parquet/orc/pickle can we remove them and any catalogs/sample files that are not supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sherifnada we support parquet and pickle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pickle works if it contains DataFrame. Because read_pickle() method returns same type as object stored in file. And discover and read methods of client expect for DataFrame.
@@ -34,6 +34,12 @@ | |||
"paramiko==2.7.2", | |||
"s3fs==0.5.2", | |||
"smart-open[all]==4.1.2", | |||
"lxml==4.6.2", | |||
"html5lib==1.1", | |||
"beautifulsoup4==4.9.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are all of these libs required? html/BS/pyarrow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This libraries required for read different file formats.
@vitaliizazmic once you've addressed the comments could you:
I'll publish and merge. Thanks! |
@@ -66,13 +66,13 @@ File Formats are mostly enabled \(and further tested\) thanks to other open-sour | |||
| Format | Supported? | | |||
| :--- | :--- | | |||
| CSV | Yes | | |||
| JSON | Experimental | | |||
| HTML | Untested | | |||
| JSON | Yes | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally put it as 'Experimental' because Source-File couldn't handle nested JSON files
(flat JSON file worked)
/publish connector=connectors=source-file
|
…92_test_file_formats
/publish connector=connectors=source-file
|
/publish connector=connectors/source-file
|
/test connector=source-file
|
@vitaliizazmic tests are failing, can you take a look ? |
…' into vitalii/1392_test_file_formats
/test connector=source-file
|
/publish connector=connectors/source-file
|
How
Pre-merge Checklist